Conversation
Add 7 test cases covering default privilege operations: - add_table_privilege: GRANT SELECT/INSERT/UPDATE on tables - add_sequence_privilege: GRANT USAGE/SELECT on sequences - add_function_privilege: REVOKE/GRANT EXECUTE on functions - add_type_privilege: GRANT USAGE on types - add_privilege_with_grant_option: WITH GRANT OPTION support - drop_privilege: remove all default privileges - alter_privilege: modify existing privileges 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds support for PostgreSQL default privileges, enabling pgschema to track, compare, and generate migration scripts for ALTER DEFAULT PRIVILEGES statements. This addresses part of issue #227 by implementing the infrastructure to detect and manage default privileges for tables, sequences, functions, and types.
Key changes:
- Added
DefaultPrivilegetype to the IR (Intermediate Representation) with support for object types (TABLES, SEQUENCES, FUNCTIONS, TYPES), grantees, privilege lists, and grant options - Implemented database queries to extract default privileges from
pg_default_acl - Created diff logic to detect added, dropped, and modified default privileges with appropriate SQL generation (GRANT/REVOKE statements)
Reviewed changes
Copilot reviewed 44 out of 49 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
ir/ir.go |
Defines the DefaultPrivilege struct and related types to represent default privilege settings in the IR |
ir/inspector.go |
Adds buildDefaultPrivileges function to extract and group default privileges from the database |
ir/queries/queries.sql |
Adds SQL query to retrieve default privileges from pg_default_acl system catalog |
ir/queries/queries.sql.go |
Generated Go code for the default privileges query |
internal/diff/diff.go |
Adds diff type for default privileges and comparison logic to detect changes |
internal/diff/default_privilege.go |
Implements SQL generation for GRANT/REVOKE default privilege statements |
internal/dump/formatter.go |
Adds "default_privileges" directory for multi-file output organization |
testdata/diff/privilege/* |
Test cases for various default privilege scenarios (drop, alter, add for different object types) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,10 @@ | |||
| Plan: 2 to drop. | |||
There was a problem hiding this comment.
The plan summary states "Plan: 2 to drop" which is incorrect. Based on the changes from old.sql to new.sql:
- 1 default privilege needs to be dropped (USAGE on SEQUENCES)
- 1 default privilege needs to be altered (adding INSERT and UPDATE to existing SELECT on TABLES)
The summary should reflect both drop and alter operations, not just "2 to drop".
internal/diff/default_privilege.go
Outdated
| statements = append(statements, fmt.Sprintf("ALTER DEFAULT PRIVILEGES IN SCHEMA %s REVOKE %s ON %s FROM %s;", | ||
| targetSchema, strings.Join(toRevoke, ", "), d.Old.ObjectType, grantee)) | ||
| } | ||
|
|
||
| // Generate GRANT for added privileges | ||
| if len(toGrant) > 0 { | ||
| sort.Strings(toGrant) | ||
| sql := fmt.Sprintf("ALTER DEFAULT PRIVILEGES IN SCHEMA %s GRANT %s ON %s TO %s", | ||
| targetSchema, strings.Join(toGrant, ", "), d.New.ObjectType, grantee) | ||
| if d.New.WithGrantOption { | ||
| sql += " WITH GRANT OPTION" | ||
| } | ||
| statements = append(statements, sql+";") | ||
| } | ||
|
|
||
| // Handle WITH GRANT OPTION changes (if privileges are same but grant option changed) | ||
| if len(toRevoke) == 0 && len(toGrant) == 0 && d.Old.WithGrantOption != d.New.WithGrantOption { | ||
| // Need to revoke and re-grant with new option | ||
| sortedPrivs := make([]string, len(d.New.Privileges)) | ||
| copy(sortedPrivs, d.New.Privileges) | ||
| sort.Strings(sortedPrivs) | ||
| privStr := strings.Join(sortedPrivs, ", ") | ||
|
|
||
| // Revoke first | ||
| statements = append(statements, fmt.Sprintf("ALTER DEFAULT PRIVILEGES IN SCHEMA %s REVOKE %s ON %s FROM %s;", | ||
| targetSchema, privStr, d.New.ObjectType, grantee)) | ||
|
|
||
| // Then grant with correct option | ||
| sql := fmt.Sprintf("ALTER DEFAULT PRIVILEGES IN SCHEMA %s GRANT %s ON %s TO %s", | ||
| targetSchema, privStr, d.New.ObjectType, grantee) | ||
| if d.New.WithGrantOption { | ||
| sql += " WITH GRANT OPTION" | ||
| } | ||
| statements = append(statements, sql+";") |
There was a problem hiding this comment.
The schema name and grantee (role name) in these ALTER DEFAULT PRIVILEGES statements are not being quoted using ir.QuoteIdentifier(). This could cause SQL errors if the schema or role name contains uppercase letters, special characters, spaces, or matches PostgreSQL reserved words.
Consider updating the code to use ir.QuoteIdentifier() for both targetSchema and grantee in all statements within this function.
| // Find modified default privileges | ||
| for key, newDP := range newDefaultPrivs { | ||
| if oldDP, exists := oldDefaultPrivs[key]; exists { | ||
| if !defaultPrivilegesEqual(oldDP, newDP) { | ||
| diff.modifiedDefaultPrivileges = append(diff.modifiedDefaultPrivileges, &defaultPrivilegeDiff{ | ||
| Old: oldDP, | ||
| New: newDP, | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The modifiedDefaultPrivileges slice is not being sorted for deterministic output, unlike addedDefaultPrivileges and droppedDefaultPrivileges which are sorted on lines 954-965. This could lead to non-deterministic diff output when multiple default privileges are modified.
Consider adding a sort for modifiedDefaultPrivileges similar to the other slices, for example after line 965.
| @@ -0,0 +1 @@ | |||
| No changes detected. | |||
There was a problem hiding this comment.
The test data files are inconsistent. The diff.sql file shows that a GRANT statement should be generated, but plan.txt states "No changes detected" and plan.json has groups set to null.
These files should be updated to match the expected diff output in diff.sql, indicating that 1 default privilege should be created.
| No changes detected. | |
| 1 default privilege will be created. |
|
|
||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE USAGE ON SEQUENCES FROM app_user; | ||
|
|
||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user; |
There was a problem hiding this comment.
The DDL statement on line 10 is incorrect. It shows:
"ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user;"
However, based on the old.sql and new.sql files, the correct statement should be:
"ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user;"
This is because new.sql adds INSERT and UPDATE privileges while keeping SELECT (it doesn't remove SELECT).
| @@ -0,0 +1,3 @@ | |||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE USAGE ON SEQUENCES FROM app_user; | |||
|
|
|||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user; | |||
There was a problem hiding this comment.
The plan.sql file does not match the expected diff output based on the old.sql and new.sql files.
Looking at the changes:
- old.sql grants SELECT and USAGE privileges
- new.sql grants SELECT, INSERT, UPDATE (no USAGE)
The correct diff should be:
- REVOKE USAGE ON SEQUENCES FROM app_user (removing USAGE privilege)
- GRANT INSERT, UPDATE ON TABLES TO app_user (adding INSERT and UPDATE, SELECT already exists)
However, this plan.sql shows:
- REVOKE USAGE ON SEQUENCES FROM app_user (correct)
- REVOKE SELECT ON TABLES FROM app_user (incorrect - should be GRANT INSERT, UPDATE)
The diff.sql file has the correct expected output. Please update plan.sql to match diff.sql.
| { | ||
| "sql": "ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE SELECT ON TABLES FROM app_user;", | ||
| "type": "default_privilege", | ||
| "operation": "drop", | ||
| "path": "default_privileges.TABLES.app_user" |
There was a problem hiding this comment.
The plan.json does not match the expected diff output. Line 18 shows a REVOKE SELECT statement which is incorrect. Based on old.sql and new.sql, this should be a GRANT statement for INSERT and UPDATE privileges instead.
The correct statement should be:
"ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT INSERT, UPDATE ON TABLES TO app_user;"
This should also have operation "alter" or "create" instead of "drop".
| for key, privs := range grouped { | ||
| dp := &DefaultPrivilege{ | ||
| ObjectType: DefaultPrivilegeObjectType(key.ObjectType), | ||
| Grantee: key.Grantee, | ||
| Privileges: privs, | ||
| WithGrantOption: key.WithGrantOption, | ||
| } | ||
| defaultPrivileges = append(defaultPrivileges, dp) |
There was a problem hiding this comment.
The privileges array should be sorted before being assigned to ensure deterministic output. Currently, the order of privileges within each DefaultPrivilege struct depends on the order they were retrieved from the database and added to the map, which may not be consistent.
Consider adding sort.Strings(privs) before line 1905 to ensure the privileges are always in alphabetical order, matching the behavior in generateGrantDefaultPrivilegeSQL and generateRevokeDefaultPrivilegeSQL which sort privileges before generating SQL.
| @@ -0,0 +1 @@ | |||
| No changes detected. | |||
There was a problem hiding this comment.
The test data files are inconsistent. The diff.sql file shows that a GRANT statement should be generated, but:
- plan.txt states "No changes detected" (should indicate 1 change to create/add)
- plan.sql is empty (should contain the GRANT statement from diff.sql)
- plan.json has groups set to null (should have the GRANT operation)
These files should be updated to match the expected diff output in diff.sql.
| No changes detected. | |
| 1 change detected. |
| @@ -0,0 +1 @@ | |||
| No changes detected. | |||
There was a problem hiding this comment.
The test data files are inconsistent. The diff.sql file shows that a GRANT statement should be generated, but plan.txt states "No changes detected" and plan.json has groups set to null.
These files should be updated to match the expected diff output in diff.sql, indicating that 1 default privilege should be created.
Add support for ALTER DEFAULT PRIVILEGES to manage default access privileges for future objects created in a schema. Implementation: - Add DefaultPrivilege struct to IR with ObjectType, Grantee, Privileges, and WithGrantOption fields - Query pg_default_acl system catalog using aclexplode() to extract individual privilege grants - Generate GRANT/REVOKE statements for default privilege changes - Support TABLES, SEQUENCES, FUNCTIONS, and TYPES object types - Support WITH GRANT OPTION for delegated privilege management Test cases cover: - Adding default privileges for each object type - Modifying existing default privileges - Removing default privileges - WITH GRANT OPTION handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1a01e57 to
0da4dde
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| END $$; | ||
|
|
||
| -- Grant USAGE, SELECT on future sequences to app_user | ||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT USAGE, SELECT ON SEQUENCES TO app_user; |
There was a problem hiding this comment.
The privilege order in the source SQL file does not match the expected output in the plan files. The new.sql specifies "USAGE, SELECT" but plan.sql expects "SELECT, USAGE" (alphabetically sorted). While the code correctly sorts privileges alphabetically at lines 66-68, the test data should be consistent with the expected output to avoid confusion.
| END $$; | ||
|
|
||
| -- Grant with grant option - admin_user can grant to others | ||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public GRANT SELECT, INSERT ON TABLES TO admin_user WITH GRANT OPTION; |
There was a problem hiding this comment.
The privilege order in the source SQL file does not match the expected output in the plan files. The new.sql specifies "SELECT, INSERT" but plan.sql expects "INSERT, SELECT" (alphabetically sorted). While the code correctly sorts privileges alphabetically, the test data should be consistent with the expected output to avoid confusion.
| WithGrantOption: p.IsGrantable.Valid && p.IsGrantable.Bool, | ||
| } | ||
|
|
||
| grouped[key] = append(grouped[key], p.PrivilegeType.String) |
There was a problem hiding this comment.
The privileges array is not sorted when building the DefaultPrivilege objects. While the privileges are sorted later during SQL generation (in generateGrantDefaultPrivilegeSQL), storing them in a deterministic order would improve consistency and make the IR representation more predictable. Consider sorting the privileges array before assigning it to the DefaultPrivilege struct at line 1905.
| @@ -0,0 +1,3 @@ | |||
| ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE USAGE ON SEQUENCES FROM app_user; | |||
There was a problem hiding this comment.
There is no test case covering the scenario where both privileges and WithGrantOption change simultaneously. This would help validate the fix for the bug in generateAlterDefaultPrivilegeStatements. Consider adding a test case where, for example, old has [SELECT] WITH GRANT OPTION and new has [SELECT, INSERT] WITHOUT GRANT OPTION.
- Sort privileges at extraction time in inspector.go for deterministic IR - Fix bug where unchanged privileges didn't get their grant option updated when both privileges and WithGrantOption changed simultaneously - Add test case for ALTER DEFAULT PRIVILEGES with combined changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 58 out of 58 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add TypeDefaultPrivilege constant to plan.go - Add default privileges to getObjectOrder() for proper display order - Normalize underscore-separated type names to space-separated format to match Type constants (e.g., "default_privileges" → "default privileges") - Update test expected files with correct summary output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove unnecessary database objects (tables, functions, sequences, types) from privilege test cases. Default privileges only affect future objects, so existing objects were not needed for testing ALTER DEFAULT PRIVILEGES. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 59 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| WHEN 'S' THEN 'SEQUENCES' | ||
| WHEN 'f' THEN 'FUNCTIONS' | ||
| WHEN 'T' THEN 'TYPES' | ||
| WHEN 'n' THEN 'SCHEMAS' |
There was a problem hiding this comment.
The SQL query includes a mapping for schema object type ('n' -> 'SCHEMAS') but there is no corresponding constant defined in the DefaultPrivilegeObjectType enum. This could cause a type mismatch if PostgreSQL returns default privileges for schemas. Either add a constant for SCHEMAS or filter out schema privileges in the query.
| // Compare default privileges across all schemas | ||
| oldDefaultPrivs := make(map[string]*ir.DefaultPrivilege) | ||
| newDefaultPrivs := make(map[string]*ir.DefaultPrivilege) | ||
|
|
||
| // Extract default privileges from all schemas in oldIR | ||
| for _, dbSchema := range oldIR.Schemas { | ||
| for _, dp := range dbSchema.DefaultPrivileges { | ||
| key := string(dp.ObjectType) + ":" + dp.Grantee | ||
| oldDefaultPrivs[key] = dp | ||
| } | ||
| } | ||
|
|
||
| // Extract default privileges from all schemas in newIR | ||
| for _, dbSchema := range newIR.Schemas { | ||
| for _, dp := range dbSchema.DefaultPrivileges { | ||
| key := string(dp.ObjectType) + ":" + dp.Grantee | ||
| newDefaultPrivs[key] = dp | ||
| } | ||
| } |
There was a problem hiding this comment.
The key used to compare default privileges does not include the schema name, which could cause collisions when default privileges are defined in multiple schemas. If the same ObjectType and Grantee combination exists in different schemas with different privileges, the map will only retain the last one encountered, leading to incorrect diff results. The key should include the schema name to ensure uniqueness across schemas.
* test: add test cases for ALTER DEFAULT PRIVILEGES support Add 7 test cases covering default privilege operations: - add_table_privilege: GRANT SELECT/INSERT/UPDATE on tables - add_sequence_privilege: GRANT USAGE/SELECT on sequences - add_function_privilege: REVOKE/GRANT EXECUTE on functions - add_type_privilege: GRANT USAGE on types - add_privilege_with_grant_option: WITH GRANT OPTION support - drop_privilege: remove all default privileges - alter_privilege: modify existing privileges 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * feat: support ALTER DEFAULT PRIVILEGES Add support for ALTER DEFAULT PRIVILEGES to manage default access privileges for future objects created in a schema. Implementation: - Add DefaultPrivilege struct to IR with ObjectType, Grantee, Privileges, and WithGrantOption fields - Query pg_default_acl system catalog using aclexplode() to extract individual privilege grants - Generate GRANT/REVOKE statements for default privilege changes - Support TABLES, SEQUENCES, FUNCTIONS, and TYPES object types - Support WITH GRANT OPTION for delegated privilege management Test cases cover: - Adding default privileges for each object type - Modifying existing default privileges - Removing default privileges - WITH GRANT OPTION handling 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: handle simultaneous privilege and grant option changes - Sort privileges at extraction time in inspector.go for deterministic IR - Fix bug where unchanged privileges didn't get their grant option updated when both privileges and WithGrantOption changed simultaneously - Add test case for ALTER DEFAULT PRIVILEGES with combined changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: show default privileges in plan summary by type - Add TypeDefaultPrivilege constant to plan.go - Add default privileges to getObjectOrder() for proper display order - Normalize underscore-separated type names to space-separated format to match Type constants (e.g., "default_privileges" → "default privileges") - Update test expected files with correct summary output 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * test: simplify privilege test cases Remove unnecessary database objects (tables, functions, sequences, types) from privilege test cases. Default privileges only affect future objects, so existing objects were not needed for testing ALTER DEFAULT PRIVILEGES. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Address part of #227